-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(slo): new slo architecture #172224
feat(slo): new slo architecture #172224
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
037095c
to
51caec2
Compare
1801e01
to
f3a8c11
Compare
@kdelemme Thanks for clear explanation regarding cloning the SLO definition and not the SLO instance. And you are right, with the new flow for cloning an SLO this will become less problematic, user can change before saving the cloned SLO. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, didn't manage to break it
"slo.name": Object { | ||
"script": Object { | ||
"source": "emit('irrelevant')", | ||
}, | ||
"type": "keyword", | ||
}, | ||
"slo.objective.target": Object { | ||
"script": Object { | ||
"source": "emit(0.999)", | ||
}, | ||
"type": "double", | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of remaining runtime_mappings
in the source index?
i am still seeing slo.id
slo.revision
and slo.instanceId
in runtime_mappings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we are injecting this field through runtime_mapping, so we can groupBy on them in the aggregation. The source data does not have the slo.id, revision etc, so we have to inject them in order to group on them :) We asked the transform team to provide a more efficient way of doing this, e.g. "static fields"
...gins/observability/server/services/slo/summary_transform_generator/generators/occurrences.ts
Outdated
Show resolved
Hide resolved
...gins/observability/server/services/slo/summary_transform_generator/generators/occurrences.ts
Outdated
Show resolved
Hide resolved
query: { | ||
bool: { | ||
filter: [ | ||
{ | ||
range: { | ||
'@timestamp': { | ||
gte: `now-${slo.timeWindow.duration.format()}/m`, | ||
lte: 'now/m', | ||
}, | ||
}, | ||
}, | ||
{ | ||
term: { | ||
'slo.id': slo.id, | ||
}, | ||
}, | ||
{ | ||
term: { | ||
'slo.revision': slo.revision, | ||
}, | ||
}, | ||
], | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wonder if it makes sense to specify sorting on search here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you sort on? And I think the transform takes care of that with the date histogram and other group by?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM !!
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Canvas Sharable Runtime
Page load bundle
Saved Objects .kibana field count
Unknown metric groupsAPI count
async chunk count
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
(cherry picked from commit b51304f)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
# Backport This will backport the following commits from `main` to `8.12`: - [feat(slo): new slo architecture (#172224)](#172224) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Kevin Delemme","email":"kevin.delemme@elastic.co"},"sourceCommit":{"committedDate":"2023-12-12T13:45:12Z","message":"feat(slo): new slo architecture (#172224)","sha":"b51304f3f3c3e8510c44a235d0fc65c44fcce225","branchLabelMapping":{"^v8.13.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:breaking","backport:prev-minor","ci:build-serverless-image","Feature:SLO","v8.12.0","Team:obs-ux-management","v8.13.0"],"number":172224,"url":"https://github.com/elastic/kibana/pull/172224","mergeCommit":{"message":"feat(slo): new slo architecture (#172224)","sha":"b51304f3f3c3e8510c44a235d0fc65c44fcce225"}},"sourceBranch":"main","suggestedTargetBranches":["8.12"],"targetPullRequestStates":[{"branch":"8.12","label":"v8.12.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.13.0","labelRegex":"^v8.13.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/172224","number":172224,"mergeCommit":{"message":"feat(slo): new slo architecture (#172224)","sha":"b51304f3f3c3e8510c44a235d0fc65c44fcce225"}}]}] BACKPORT--> Co-authored-by: Kevin Delemme <kevin.delemme@elastic.co>
## Summary This PR is a follow up to #172224, it adds a UI for resetting the SLO definitions from the previous model. Once #179473 is merged I will rebase this against `main` and convert it from a "draft" PR to "ready to review". ![image](https://github.com/elastic/kibana/assets/41702/daf0591c-272f-40c2-9831-658d7b9b1378) ![image](https://github.com/elastic/kibana/assets/41702/d385396d-d840-4574-942a-b8e51ce66066) ![image](https://github.com/elastic/kibana/assets/41702/729df2a0-61e6-41b3-aaa5-8501e7aa7797) ### Testing 1. Start by loading `main` 2. Ingest some data 3. Create some SLOs 4. Change Kibana from `main` to this PR 5. Visit the SLO page, you should see a banner similar to the screen shot above. 6. Do your best to break this --------- Co-authored-by: shahzad31 <shahzad31comp@gmail.com> Co-authored-by: Dominique Clarke <doclarke71@gmail.com>
## Summary This PR is a follow up to elastic#172224, it adds a UI for resetting the SLO definitions from the previous model. Once elastic#179473 is merged I will rebase this against `main` and convert it from a "draft" PR to "ready to review". ![image](https://github.com/elastic/kibana/assets/41702/daf0591c-272f-40c2-9831-658d7b9b1378) ![image](https://github.com/elastic/kibana/assets/41702/d385396d-d840-4574-942a-b8e51ce66066) ![image](https://github.com/elastic/kibana/assets/41702/729df2a0-61e6-41b3-aaa5-8501e7aa7797) ### Testing 1. Start by loading `main` 2. Ingest some data 3. Create some SLOs 4. Change Kibana from `main` to this PR 5. Visit the SLO page, you should see a banner similar to the screen shot above. 6. Do your best to break this --------- Co-authored-by: shahzad31 <shahzad31comp@gmail.com> Co-authored-by: Dominique Clarke <doclarke71@gmail.com> (cherry picked from commit c2003d9)
# Backport This will backport the following commits from `main` to `8.12`: - [[SLO] Reset UI for updating outdated SLOs (#172883)](#172883) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Chris Cowan","email":"chris@elastic.co"},"sourceCommit":{"committedDate":"2023-12-12T19:36:20Z","message":"[SLO] Reset UI for updating outdated SLOs (#172883)\n\n## Summary\r\n\r\nThis PR is a follow up to #172224, it adds a UI for resetting the SLO\r\ndefinitions from the previous model. Once #179473 is merged I will\r\nrebase this against `main` and convert it from a \"draft\" PR to \"ready to\r\nreview\".\r\n\r\n\r\n![image](https://github.com/elastic/kibana/assets/41702/daf0591c-272f-40c2-9831-658d7b9b1378)\r\n\r\n\r\n![image](https://github.com/elastic/kibana/assets/41702/d385396d-d840-4574-942a-b8e51ce66066)\r\n\r\n\r\n![image](https://github.com/elastic/kibana/assets/41702/729df2a0-61e6-41b3-aaa5-8501e7aa7797)\r\n\r\n\r\n### Testing\r\n\r\n1. Start by loading `main`\r\n2. Ingest some data\r\n3. Create some SLOs\r\n4. Change Kibana from `main` to this PR\r\n5. Visit the SLO page, you should see a banner similar to the screen\r\nshot above.\r\n6. Do your best to break this\r\n\r\n---------\r\n\r\nCo-authored-by: shahzad31 <shahzad31comp@gmail.com>\r\nCo-authored-by: Dominique Clarke <doclarke71@gmail.com>","sha":"c2003d9f83f6d437ec9ce46943a402b38c07ece5","branchLabelMapping":{"^v8.13.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:enhancement","backport:prev-minor","Feature:SLO","v8.12.0","Team:obs-ux-management","v8.13.0"],"number":172883,"url":"https://github.com/elastic/kibana/pull/172883","mergeCommit":{"message":"[SLO] Reset UI for updating outdated SLOs (#172883)\n\n## Summary\r\n\r\nThis PR is a follow up to #172224, it adds a UI for resetting the SLO\r\ndefinitions from the previous model. Once #179473 is merged I will\r\nrebase this against `main` and convert it from a \"draft\" PR to \"ready to\r\nreview\".\r\n\r\n\r\n![image](https://github.com/elastic/kibana/assets/41702/daf0591c-272f-40c2-9831-658d7b9b1378)\r\n\r\n\r\n![image](https://github.com/elastic/kibana/assets/41702/d385396d-d840-4574-942a-b8e51ce66066)\r\n\r\n\r\n![image](https://github.com/elastic/kibana/assets/41702/729df2a0-61e6-41b3-aaa5-8501e7aa7797)\r\n\r\n\r\n### Testing\r\n\r\n1. Start by loading `main`\r\n2. Ingest some data\r\n3. Create some SLOs\r\n4. Change Kibana from `main` to this PR\r\n5. Visit the SLO page, you should see a banner similar to the screen\r\nshot above.\r\n6. Do your best to break this\r\n\r\n---------\r\n\r\nCo-authored-by: shahzad31 <shahzad31comp@gmail.com>\r\nCo-authored-by: Dominique Clarke <doclarke71@gmail.com>","sha":"c2003d9f83f6d437ec9ce46943a402b38c07ece5"}},"sourceBranch":"main","suggestedTargetBranches":["8.12"],"targetPullRequestStates":[{"branch":"8.12","label":"v8.12.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.13.0","labelRegex":"^v8.13.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/172883","number":172883,"mergeCommit":{"message":"[SLO] Reset UI for updating outdated SLOs (#172883)\n\n## Summary\r\n\r\nThis PR is a follow up to #172224, it adds a UI for resetting the SLO\r\ndefinitions from the previous model. Once #179473 is merged I will\r\nrebase this against `main` and convert it from a \"draft\" PR to \"ready to\r\nreview\".\r\n\r\n\r\n![image](https://github.com/elastic/kibana/assets/41702/daf0591c-272f-40c2-9831-658d7b9b1378)\r\n\r\n\r\n![image](https://github.com/elastic/kibana/assets/41702/d385396d-d840-4574-942a-b8e51ce66066)\r\n\r\n\r\n![image](https://github.com/elastic/kibana/assets/41702/729df2a0-61e6-41b3-aaa5-8501e7aa7797)\r\n\r\n\r\n### Testing\r\n\r\n1. Start by loading `main`\r\n2. Ingest some data\r\n3. Create some SLOs\r\n4. Change Kibana from `main` to this PR\r\n5. Visit the SLO page, you should see a banner similar to the screen\r\nshot above.\r\n6. Do your best to break this\r\n\r\n---------\r\n\r\nCo-authored-by: shahzad31 <shahzad31comp@gmail.com>\r\nCo-authored-by: Dominique Clarke <doclarke71@gmail.com>","sha":"c2003d9f83f6d437ec9ce46943a402b38c07ece5"}}]}] BACKPORT--> Co-authored-by: Chris Cowan <chris@elastic.co> Co-authored-by: Dominique Clarke <dominique.clarke@elastic.co>
🍒 Summary
This PR introduces a breaking change in the SLO feature. We are moving away from global summary transforms (there were 10 of them), and instead install the following resources per SLO definition:
slo-{id}-{revision}
slo-summary-{id}-{revision}
.slo-observability.summary.pipeline-{id}-{revision}
The rollup transform is setup with a common rollup ingest pipeline
.slo-observability.sli.pipeline-v${SLO_RESOURCES_VERSION}
which setsevent.ingested
timestamp as well as splitting the destination index per month (as we were already doing before). This newevent.ingested
field in the rollup data is used to sync the summary transforms with. This should prevent any issue with delays coming from the rollup transform, e.g. slow queries from ccs.The summary transform uses simpler queries and less groupBy fields which should make the transform runs faster.
The summary ingest pipeline sets the SLO definition metadata when the transform update the summary document.
Summary documents are becoming space aware: this fixes a bug when SLOs are created in different spaces, but returned from the summary search client before being filtered out, resulting in erroneous pagination.
🧪 Testing
Upgrading kibana from main to this branch
When starting kibana on the new branch after having started kibana on main, the new resources (indices, pipeline) should be created successfully.
Any existing SLO should have their SO migrated (version added in the saved object definition).
Any existing SLO should be reset-able through the API:
POST /api/observability/slos/{id}/_reset
and should appear on the SLO listing page again.Restarting Kibana
When restarting kibana, the resources installation should not fail.
Updating an SLO
If the change is made on a field requiring a revision bump, e.g.
indicator, timeWindow, budgetingMethod, settings, objective
, therevision
will be bump, and a two new transforms (rollup and summary) for that new revision should be created, while the two previous transforms should be deleted.SLO space aware
Two SLOs created in different spaces should not be shown in the other space, the
total
in the paginated response should be 1 (for one SLO in that space)Creating a serverless deployment
Set your API_KEY and ENV_URL, then create a new deployment
Update an existing deployment (find the deployment_id either in the previous reponse, or in the cloud console) with this PR image:
or you can create a serverless deployment directly with that PR image:
Run the High cardinality indexer:
Release note
We introduce a breaking change in the SLO features that will break any SLOs created before 8.12. These SLOs will have to be manually reseted through an API until we provide a UI for it. The data aggregated over time (rollup) will still be available in the sli v2 index, but won't be used for summary calculation when reset.
The previous summary transforms summarizing every SLOs won't be used anymore and can be stopped and deleted:
Be aware that when installing a new SLO (or after resetting an SLO), we install two transforms (one for the rollup data and one that summarize the rollup data). Do not delete the new
slo-summary-{slo_id}-{slo_revision}
transforms.